-
Notifications
You must be signed in to change notification settings - Fork 747
fix(amazonq): Bug fixes for LSP auth on agentic mode #7231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(amazonq): Bug fixes for LSP auth on agentic mode #7231
Conversation
|
| if ( | ||
| (Experiments.instance.get('amazonqLSP', true) || AuthUtil.instance.isInternalAmazonUser()) && | ||
| (!isAmazonInternalOs() || (await hasGlibcPatch())) | ||
| ) { | ||
| // start the Amazon Q LSP for internal users first | ||
| // for AL2, start LSP if glibc patch is found | ||
| await activateAmazonqLsp(context) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup doesn't work with auth on LSP, since the LSP needs to start for auth to work
| context.subscriptions.push(amazonq.focusAmazonQPanel.register(), amazonq.focusAmazonQPanelKeybinding.register()) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now part of
| amazonq.focusAmazonQPanel.register(), |
| const installResult = await new AmazonQLspInstaller().resolve() | ||
| return await lspSetupStage('launch', () => startLanguageServer(ctx, installResult.resourcePaths)) | ||
| }) | ||
| AuthUtil.create(new auth2.LanguageClientAuth(client, clientId, encryptionKey)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move instantiation of the AuthUtil singleton into startLanguageServer. The function uses several AuthUtil calls, which fail if it is not instantiated yet
| if (uri.scheme.startsWith('http')) { | ||
| try { | ||
| await openUrl(vscode.Uri.parse(params.uri)) | ||
| return params | ||
| } catch (err: any) { | ||
| getLogger().error(`Failed to open http from LSP: error: %s`, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality was added to enable LSP to open documents in agentic chat. We were relying on the same request type to open the auth URL we receive from LSP in the login flow
| if (!this.isConnected()) { | ||
| await this.refreshState() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that a session is restored successfully upon start of the extension with expired SSO. If SSO is expired upon start of extension, the LSP throws an error. Since there is no state change in that case (notConnected > notConnected), we can't rely on the state change handler to trigger a login flow
|
| notifications: true, | ||
| showSaveFileDialog: true, | ||
| }, | ||
| q: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this can be dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The merge with mainline made it duplicate, see line 140
8787af3
into
aws:feature/amazonqLSP-auth
Problem
Agentic chat introduced code changes that break auth on LSP
Solution
Fixes for the auth flow:
AuthUtilintostartLanguageServersinc ethe function uses several AuthUtil calls, which fail if it is not instantiated yetShowDocumentRequesthandler, and remove the old handler fromclient.ts.feature/xbranches will not be squash-merged at release time.